-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_staking: align the mock staking contract interface with the real contract #11325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
apollo_staking: align the mock staking contract interface with the real contract #11325
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4ec0035 to
308b0f6
Compare
de9d1e1 to
80d48d6
Compare
matanl-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matanl-starkware reviewed 9 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry).
crates/apollo_staking/src/contract_types.rs line 59 at r2 (raw file):
impl TryFromIterator<Felt> for ContractStaker { type Error = RetdataDeserializationError; fn try_from_iter<T: Iterator<Item = Felt>>(iter: &mut T) -> Result<Self, Self::Error> {
Worth adding a comment describing the how the Felts supposed to be when everything is OK
Code quote:
fn try_from_itercrates/apollo_staking/src/contract_types.rs line 124 at r2 (raw file):
})?; let mut result = Vec::new();
Reserve capacity
Code quote:
Vec::new()crates/apollo_staking/src/contract_types.rs line 156 at r2 (raw file):
#[cfg(test)] impl From<&Staker> for ContractStaker {
Why do we need the type Staker?
Code quote:
Staker80d48d6 to
d1a01a5
Compare
dafnamatsry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dafnamatsry made 3 comments.
Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @matanl-starkware).
crates/apollo_staking/src/contract_types.rs line 59 at r2 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Worth adding a comment describing the how the Felts supposed to be when everything is OK
Done.
crates/apollo_staking/src/contract_types.rs line 124 at r2 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Reserve capacity
Done.
crates/apollo_staking/src/contract_types.rs line 156 at r2 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Why do we need the type
Staker?
It simplifies the get_committee interface by removing the Option from public key.
d1a01a5 to
d5715dc
Compare
matanl-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matanl-starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry).
crates/apollo_staking/src/contract_types.rs line 156 at r2 (raw file):
Previously, dafnamatsry wrote…
It simplifies the
get_committeeinterface by removing theOptionfrom public key.
OK
So lets add also From<&ContractStaker> for Staker
d5715dc to
5196120
Compare
dafnamatsry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dafnamatsry made 1 comment.
Reviewable status: 7 of 9 files reviewed, all discussions resolved (waiting on @matanl-starkware).
crates/apollo_staking/src/contract_types.rs line 156 at r2 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
OK
So lets add alsoFrom<&ContractStaker> for Staker
Done.
matanl-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matanl-starkware reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry).
matanl-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry).
5196120 to
520948f
Compare

No description provided.